Add scaffold surface support for URIS valves#569
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #569 +/- ##
==========================================
+ Coverage 69.44% 69.53% +0.09%
==========================================
Files 181 181
Lines 34072 34162 +90
Branches 5930 5948 +18
==========================================
+ Hits 23662 23756 +94
+ Misses 10273 10268 -5
- Partials 137 138 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ktbolt
left a comment
ktbolt
left a comment
There was a problem hiding this comment.
@hanzhao2020 These changes look good !
However, there are still lots of messages printed to std::cout in uris.cpp; please replace those with DebugMsg calls.
@ktbolt Sounds good! Replaced all the URIS printed messages with |
ktbolt
left a comment
ktbolt
left a comment
There was a problem hiding this comment.
@hanzhao2020 Good !
Note that if you would like to output useful run-time statistics for uris then you could create a log file for it, something like SimulationLogger.h, that could be activated from a parameter in the solver XML file.
|
@ktbolt Thanks for the suggestion, I'll keep that in mind! |
michelebucelli
left a comment
michelebucelli
left a comment
There was a problem hiding this comment.
Thanks @hanzhao2020! I left a few comments.
My one general concern is related to the comment I made to #566. What I meant by that comment is that, as far as I can see, in principle one could already have scaffolding with the current implementation (i.e. before this PR), by processing the input surface as follows:
- define a displacement field equal to zero on the scaffold mesh;
- append the scaffold mesh and the valve mesh (just append, no need to fix the connectivity in principle);
- give the resulting surface in input to URIS.
Is there some part of this that doesn't actually work as I would think?
@michelebucelli Thank you for the review and comments! Yes, with the current implementation on the main branch, it is already possible to include a scaffold by treating it as an additional valve leaflet and providing motion data that keeps it fixed throughout the simulation. However, this approach requires the user to provide motion data for both the opening and closing phases (even though they are identical for a stationary scaffold), and the motion files must have the same number of time steps as those of the valve leaflets. This adds unnecessary effort and could be confusing for users. Also, when the scaffold is treated as another valve leaflet, the SDF is recomputed during valve opening and closing, even though the scaffold itself does not move. These computations for scaffold SDF are unnecessary and introduce a bit additional computational cost. I think supporting the scaffold as an optional input, without requiring any motion data, provides a cleaner and more user-friendly interface while also avoiding these unnecessary SDF updates. |
michelebucelli
left a comment
michelebucelli
left a comment
There was a problem hiding this comment.
Thanks @hanzhao2020! A couple more suggestions. Other than that looks good to me!
| throw std::runtime_error("Failed to read URIS scaffold mesh for '" + uris_obj.name + "': " + e.what()); | ||
| } catch (...) { | ||
| throw std::runtime_error("Failed to read URIS scaffold mesh for '" + uris_obj.name + "'."); |
There was a problem hiding this comment.
I think it might be good to use the new exception framework introduced in #526 and #537 here.
I have introduced a FileNotFoundException class in #577, so maybe we can wait until that gets merged before using it here, since it seems appropriate. If so, I suggest putting a todo comment as a reminder for that (I mostly use the syntax @todo[username], as in @todo[michelebucelli], so that I can find them easily later, and if someone comes across it they easily know who to blame 😅)
| void load_scaffold_from_file(Simulation* simulation, urisType& uris_obj, | ||
| const std::string& scaffold_file_path) { |
There was a problem hiding this comment.
It seems to me that uris_obj is used by this function only to
- choose where to store the result, and
- construct the error message.
I think it would be better to have a more minimal interface, which would facilitate decoupling this function from the rest of the code.
Is mshType copy- or move-constructible? If so, this function could have the following cleaner interface:
mshType
load_scaffold_from_file(Simulation* simulation,
const std::string& scaffold_file_path,
const std::string& uris_name)Ideally, the documentation would explain that the uris_name argument is only used for the purpose of error messages (although honestly I'd remove this dependency too and assume that the user can figure out what valve is giving problems based on the file name alone).
In a similar vein, also taking the whole Simulation object in input, and as a non-const pointer, seems a lot. I see that it is only used as an argument to load_shell_mesh_from_file, which in turn forwards it to other methods, so perhaps it's not so easy to drop it altogether. However, I'd try at least to make it const, propagating the constness down the call stack.
3 participants
Resolves #566
Release Notes
Scaffold_file_pathTesting
Code of Conduct & Contributing Guidelines